Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

set theory v2 #7534

Merged
merged 3 commits into from Aug 5, 2014
Merged

set theory v2 #7534

merged 3 commits into from Aug 5, 2014

Conversation

bcoca
Copy link
Member

@bcoca bcoca commented May 25, 2014

Now it can handle non hashable items like dicts (which used to produce a TypeError)
Also it now makes each list unique before operating on them

@bcoca
Copy link
Member Author

bcoca commented May 25, 2014

this should solve issues presented in #7495, without a need for a workaround

@mpdehaan
Copy link
Contributor

Would it be possible to check the types of the arguments instead of the try/except?

@bcoca
Copy link
Member Author

bcoca commented May 26, 2014

I can check for dicts, but other types can be passed that are not
'hashable', the try/except covers all possible ones.

But considering that most of this should be comming from yaml, it should be
limited to dicts.

Brian Coca
Stultorum infinitus est numerus
0110000101110010011001010110111000100111011101000010000001111001011011110111010100100000011100110110110101100001011100100111010000100001
Pedo mellon a minno

@bcoca
Copy link
Member Author

bcoca commented May 26, 2014

actually, after thinking for 10s:

if isinstance(a, collections.Hashable) : ...

if isinstance(a,collections.Hashable) and isinstance(b,collections.Hashable):
c = set(a) | set(b)
else:
c = a + b
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice that, conceptually, the set(a) | set(b) operation is different than a + b. The former performs set union meaning that duplicates are removed, whereas the latter is a simple concatenation with the possibility for the same item to appear twice.

If these are considered "set operations", then the else part should be replaced by something like:

c = a + filter(lambda x: x not in a, b)

@bcoca
Copy link
Member Author

bcoca commented May 30, 2014

thanks, I had changed the rest, I'm not sure why I missed this one

@kilburn
Copy link
Contributor

kilburn commented May 30, 2014

Actually @mpdehaan pointed out that, by turning inputs into sets, we are removing duplicates in them too. Hence, the fallback operations are all wrong here.

Example with hashable items:

a,b = [x,x,y,y], [x,z,z]
unique(a) # set([x,y])
intersect(a, b) # set([x])
difference(a, b) # set([y])
symmetric_difference(a, b) # set([y,z])
union(a, b) # set([x,y,z])

Example with non-hashable items:

a,b = [x,x,y,y], [x,z,z]
unique(a) # set([x,x,y,y]) - Wrong: the not in c check is useless because c is populated only
                             after finishing the whole filter operation
intersect(a, b) # set([x,x]) - Wrong: duplicates in a are not removed
difference(a, b) # set([y,y]) - Wrong: duplicates in a are not removed
symmetric_difference(a, b) # set([y,y,z,z]) - Wrong: duplicates not removed from neither a nor b
union(a, b) # set([x,y,z]) # set([x,x,y,y,x,z,z]) - Wrong: duplicates not removed from neither a nor b

Also, the exact same result (than the set operations) is impossible to achieve because sets are implicitly unordered (meaning that you can get varying orders when iterating it) whereas lists are ordered by definition. Anyway, I guess this is an acceptable trade-off...

@bcoca
Copy link
Member Author

bcoca commented May 30, 2014

I'll resubmit tonight, also making changes to try to coerce always
returning a list/set when used in templated fields (with_items:).​

@mpdehaan mpdehaan added P4 and removed P3 labels Jun 1, 2014
@s0x
Copy link
Contributor

s0x commented Jul 16, 2014

This even fixes an issue when using filters in with_items (e.g. #8055)

@srgvg
Copy link
Contributor

srgvg commented Jul 16, 2014

This patch fixes #8141 for me. +1

@bcoca
Copy link
Member Author

bcoca commented Jul 25, 2014

+1 as we keep seeing people bitten by the use of non hashable objects with the set theory filters

@jimi-c jimi-c merged commit ce8c8ab into ansible:devel Aug 5, 2014
@jimi-c
Copy link
Member

jimi-c commented Aug 5, 2014

Merged, thanks!

@bcoca bcoca deleted the sets_v2 branch August 9, 2014 17:15
@ansibot ansibot added feature This issue/PR relates to a feature request. and removed feature_pull_request labels Mar 4, 2018
@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature This issue/PR relates to a feature request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants